fix(security): Don't decrypt all KMS keys by default #375
Conversation
|
Hey @antonbabenko @bryantbiggs! 👋🏻 As of now it dosen't resolve the issue to set the variable to an empty list or string as the statement block isn't dynamic and therefore generates a IAM policy statement that's not valid. A work around is to put in a random KeyId that's not in your account. |
|
We can switch to a dynamic block to all users to disable, but the default value should stay. Also, this is not a security vulnerability - it may be more permission than what's necessary but that's it |
|
One could agree with that, but with keeping the default value if violates AWS Key Management Service Best If you still want to keep the default value I can change that one back in this PR, but it should be mentioned somewhere that by keeping the default value your might put your account at risk. |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
This PR was automatically closed because of stale in 10 days |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
2 participants
Description
As of #349 a pretty large security vulnerability was introduced to the
iam-role-for-service-accounts-eksmodule that by default allows full decryption off all KMS keys within the account that it's implemented in. In the release notes it was noted as a fix and not a new feature. For the naked eye this can indicate that it's a fix in some existing feature that they might not be using, when the reality is that it opened up a backdoor to all KMS keys if you use the External Secrets role.Motivation and Context
I've changed the default here to be an empty array to focus on closing this security vulnerability first. I'll try to make some time to implement the condition object to the
kms:decryptstatement later on.Breaking Changes
This might be a breaking changes for those that have already implemented the security vulnerability and should be noted as such in the release documentation.
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request